-
Notifications
You must be signed in to change notification settings - Fork 45
Silo admin endpoints for user logout + listing tokens and sessions #8479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
109676e
to
276db6b
Compare
"modify" if "admin" on "parent_silo"; | ||
|
||
# A silo admin can list a user's tokens and sessions. | ||
"list_children" if "admin" on "parent_silo"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little funny because I'm using list_children
to determine whether you can list sessions and tokens (only self and silo admin can) but then I'm using modify
on the list itself to determine whether you can do the logout delete-all operation. It works fine, but it goes slightly against the grain of how I know it's supposed to work. We just don't have many delete all type things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that seems resonable to me, though I'm far from the most knowledgeable about how we tend to model stuff in Polar... @davepacheco might have an opinion?
c97c76d
to
10f014e
Compare
// TODO: unlike with tokens, we do not have expiration time here, | ||
// so we can't filter out expired sessions by comparing to now. In | ||
// the authn code, this works by dynamically comparing the created | ||
// and last used times against now + idle/absolute TTL. We may | ||
// have to do that here but it's kind of sad. It might be nicer to | ||
// make sessions work more like tokens and put idle and absolute | ||
// expiration time right there in the table at session create time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the major outstanding issue. I will probably make the fix a separate PR on top of this one because it's an actual change, not just an addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm, I like the idea of the proposed refactor --- generally it seems nicer to represent things as deadlines, I think. I agree that it's a big enough change that it could be done subsequently. In the meantime, might we want to cruft together a thing that filters such sessions out of the returned Vec
in Rust code?
Do you think it's worth opening an issue to track that, in addition to/instead of the TODO comment?
// TODO: does it make sense to use a single resource to represent both user | ||
// sessions and tokens? it seems silly to have two identical ones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how would that work? Is that a question you want to answer before merging this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status quo is the single resource version (I meant resource from the POV of authz). I think it would be simple to have two instead, I would just make a second one and name one tokens and one sessions, and use each as appropriate. Since that can be done any time we need to distinguish the two (we may never have to) I am inclined to solve this by deleting the comment.
"modify" if "admin" on "parent_silo"; | ||
|
||
# A silo admin can list a user's tokens and sessions. | ||
"list_children" if "admin" on "parent_silo"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that seems resonable to me, though I'm far from the most knowledgeable about how we tend to model stuff in Polar... @davepacheco might have an opinion?
// TODO: unlike with tokens, we do not have expiration time here, | ||
// so we can't filter out expired sessions by comparing to now. In | ||
// the authn code, this works by dynamically comparing the created | ||
// and last used times against now + idle/absolute TTL. We may | ||
// have to do that here but it's kind of sad. It might be nicer to | ||
// make sessions work more like tokens and put idle and absolute | ||
// expiration time right there in the table at session create time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm, I like the idea of the proposed refactor --- generally it seems nicer to represent things as deadlines, I think. I agree that it's a big enough change that it could be done subsequently. In the meantime, might we want to cruft together a thing that filters such sessions out of the returned Vec
in Rust code?
Do you think it's worth opening an issue to track that, in addition to/instead of the TODO comment?
opctx.authorize(authz::Action::Modify, authn_list).await?; | ||
|
||
use nexus_db_schema::schema::device_access_token; | ||
diesel::delete(device_access_token::table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Occurs to me we could do a soft-delete-like thing by mutating the expiration time to now
, though that feels kind of sketchy to me as the soft-delete would disturb the record of what that token was originally supposed to be... just hard deleting them is probably less goofy!
/// Delete all of user's tokens and sessions | ||
#[endpoint { | ||
method = POST, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be goofy of me: it seems a bit strange that this is a POST but the description in the API docs begins with the verb "delete". On the other hand, since there are separate resources for sessions and tokens which are both deleted in one operation by this route, I'm not sure if there's a natural way to make this be a DELETE...obviously it shouldn't be DELETE /v1/users/{id}
, but it's equally awkward to make it DELETE /v1/users/{id}/sessions
since it also deletes tokens. Gah. Okay.
Maybe it should remain a POST
but the slug could be changed to just say "logs out the user", with additional doc text following that that states what logging out means is "sessions and tokens get deleted"? I dunno. On the other hand, that just obscures what it actually does in the interest of my feeling Vaguely Weird about the use of the word "delete" in a route that isn't an HTTP DELETE
...
🤷♀️
self.datastore() | ||
.silo_user_tokens_delete(opctx, &authz_authn_list) | ||
.await?; | ||
|
||
self.datastore() | ||
.silo_user_sessions_delete(opctx, &authz_authn_list) | ||
.await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, so, it's worth noting that this is not atomic: if the silo_user_tokens_delete
query succeeds, but the silo_user_sessions_delete
query fails, we could return a 500 error and leave the user in a half-logged-in state where they have no auth tokens but any existing console sessions still exist. I don't actually know whether that's huge a problem or not: is anything else liable to get confused if it encounters a user in such a state, and how bad are the consequences of that? AFAICT we won't get permanently stuck in such a state, since this is idempotent: we can try to log the user out again and it won't matter that we've already deleted their tokens, right?
If it is an issue we could make this a transaction, so the whole logout operation atomically succeeds or fails. I don't think this is necessary but wanted to make sure we'd thought through it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about this a bit. Basically the half-deleted state is fine from an everything working POV, and it's at least in the direction of what the user wanted to happen, and they know from the error response that it didn't fully work (though it's likely they expect it not to have worked at all, unless the error indicates otherwise). I lean against a transaction but I will at least add a comment showing this is deliberate.
) { | ||
let testctx = &cptestctx.external_client; | ||
|
||
// create a user have a user ID on hand to use in the authn_as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sentence seems to be missing one or more words...
The idea here is that to disable a user's access to the system, admins first disable that user's ability to log in on the IdP side and then hit this endpoint to remove all of their existing credentials on our end. The centerpiece is the logout endpoint, but I added the endpoints for listing sessions and tokens because someone pointed out you really want to see those come back empty after logout. They're also kind of useful anyway. Then I added
user_view
just because it wouldn't make sense to have token and session list endpoints hanging off/v1/users/{user_id}
without having that defined./v1/users/{user_id}/logout
that deletes all of the user's tokens and sessionsSiloUserAuthnList
letting us authorize that action for silo admins specifically (can't use silo modify because fleet collaborator and admin get that on all silos)user_view
anduser_token_list
anduser_session_list
endpoints for symmetry and to give the admin a warm fuzzy feeling when they see that the tokens and sessions are in fact gone (also makes testing a little cleaner)